Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #1788 incorrect url_for for routes with hosts, added tests. #1789

Merged
merged 3 commits into from
Feb 21, 2020

Conversation

Tronic
Copy link
Member

@Tronic Tronic commented Feb 20, 2020

The router allows specifying different hostname for specific routes, and handles this internally by string concatenation of host and path into uri, confusing app.url_for into creating invalid URLs.

Ultimately it would be more sensible to keep URL and path separately in Router objects, but as a workaround to the mentioned bug report, this is now handled in url_for which uses any route-provided host where SERVER_NAME would otherwise be used.

Adds test_url_for.py to avoid regressions, and to allow for more comprehensive testing of the said function, which doesn't seem to be currently tested properly.

Fixes issue #1788

@sjsadowski
Copy link
Contributor

@Tronic gonna get this in ASAP - can you leave the branch up after merge, I think this needs to go in to 19.12 LTS which I'm still trying to fix. Thx!

@codecov
Copy link

codecov bot commented Feb 20, 2020

Codecov Report

Merging #1789 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1789      +/-   ##
=========================================
+ Coverage   92.08%   92.1%   +0.01%     
=========================================
  Files          23      23              
  Lines        2312    2317       +5     
  Branches      426     427       +1     
=========================================
+ Hits         2129    2134       +5     
  Misses        141     141              
  Partials       42      42
Impacted Files Coverage Δ
sanic/app.py 92.43% <100%> (+0.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 91f6aba...8bbbf8a. Read the comment docs.

@Tronic
Copy link
Member Author

Tronic commented Feb 20, 2020

I cannot reproduce that test failure on my own system, and it is not quite obvious from the output what went wrong. Is this related to this patch or a problem with the CI pipeline?

@sjsadowski
Copy link
Contributor

@Tronic test failure is py38 on windows. It's included for forward compat but it's not required because windows is best-effort support. You're golden.

@yunstanford yunstanford merged commit 861e873 into sanic-org:master Feb 21, 2020
ashleysommer added a commit to ashleysommer/sanic that referenced this pull request May 14, 2020
ahopkins added a commit that referenced this pull request Jul 29, 2020
* Cherry pick PRs to backport to 19.12LTS

Includes commits from:
#1762
#1764
#1789

* Fix type annotation issue; run black and isort

* Update Makefile

Co-authored-by: Ashley Sommer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants